-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate add_docker_metadata to ECS #9412
Migrate add_docker_metadata to ECS #9412
Conversation
alias6: true | ||
alias: true | ||
|
||
- from: docker.container.labels # TODO: How to map these? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@graphaelli Have you found a good way to migrate such fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can. I'm assuming this is user-populated, including key names? If that's the case, then the list of fields to migrate is unknowable.
Perhaps if some fields are often present by default, we can create aliases for those, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not found a good way. Can the migration assistant create those aliases dynamically? That is, iterate over the source labels and create aliases on the old indices for each one? Another option is to inform users to include both fields with an or clause in searches and aggregations.
APM has the same issue where context.tags
are moving to labels
. It may be possible there to introduce an APM UI-only concept of "tags" "labels" and make the context.tags.$key=$value or labels.$key=value
query on behalf of the user - we have not discussed this possibility but I'd be curious what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@graphaelli Indeed that is something that the migration assistant could potentially do. I think in this case it's more important for apm-server as you probably need it for the 6.x compatiblity layer and in our case it's nice to have as we can also break it in 7 and inform the user about this one. If we can use then the same feature to, that is great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think the migration assistant will be our best bet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@graphaelli Can you take this up with the Kibana team working on the migration assistant to see what they think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They've added it to their use cases for the migration assistant, there will be hooks for us to handle it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great news. Thanks for following up.
@@ -88,7 +88,7 @@ func buildDockerMetadataProcessor(cfg *common.Config, watcherConstructor docker. | |||
"field": "source", | |||
"separator": string(os.PathSeparator), | |||
"index": config.SourceIndex, | |||
"target": "docker.container.id", | |||
"target": "container.id", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was not sure why here the constant was not used. @exekias ok to change this one or could this have other side effects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uhm, probably they were introduced at different points in time, I don't see a reason for it to not use the constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, replaced it by the constant then.
@@ -166,10 +166,6 @@ func (d *addDockerMetadata) Run(event *beat.Event) (*beat.Event, error) { | |||
container := d.watcher.Container(cid) | |||
if container != nil { | |||
meta := common.MapStr{} | |||
metaIface, ok := event.Fields["docker"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this one as I felt it's not needed anymore when using Put
with the DeepUpdate. Please double check if that is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while that's true, I think that will create more unneeded allocations? this is the kind of code that runs lots of times
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I can fully follow here. You are mostly worried about the DeepUpdate
part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to merge for now. @exekias Let's revisit this next year to make sure we have the correct optimisations in place.
Pinging @elastic/infrastructure |
d02919b
to
309df10
Compare
0e4e3d3
to
d93cca7
Compare
I suggest we move forward with this PR even though alias for docker.container.labels do not exist as it's not straight forward. I added a note about it to our ECS issue so we can track it there. |
b671461
to
275de21
Compare
0dbc2f5
to
90746ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small issue in ecs-migration.yml, and two comments that will help inform next steps.
alias6: true | ||
alias: true | ||
|
||
- from: docker.container.labels # TODO: How to map these? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can. I'm assuming this is user-populated, including key names? If that's the case, then the list of fields to migrate is unknowable.
Perhaps if some fields are often present by default, we can create aliases for those, though.
ce274f3
to
763e16b
Compare
@webmat Could you take a look again to get this in? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
763e16b
to
c66ed45
Compare
auditbeat/docs/modules_list.asciidoc
Outdated
@@ -8,5 +8,6 @@ This file is generated! See scripts/docs_collector.py | |||
|
|||
-- | |||
|
|||
include::./modules/.DS_Store.asciidoc[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d32ddf1
to
cb8f556
Compare
cb8f556
to
d2e260d
Compare
d2e260d
to
3578b0f
Compare
Migrate the docker fields to ECS container fields. * docker.container.id -> container.id * docker.container.image -> container.image.name * docker.container.name -> container.name * docker.container.labels -> container.labels make image fix update generator script
3578b0f
to
0a3caab
Compare
Fields injected by docker autodiscover provider were being placed in alias fields introduced for ECS, change them to the new location and add selectors accordingly. This PR includes #10862 and #10758 As a summary: * Autodiscover selectors using ECS structure are added to autodiscover events, old selectors are kept for backwards compatibility * Autodiscover generated metadata follows ECS * Dedotting of labels is added, enabled by default, will be backported for 6.7, but disabled `docker.containers.labels` is not migrated, as it wasn't for `add_docker_metadata` (see #9412) Fixes #10757 Co-Authored-By: kaiyan-sheng <kaiyan.sheng@elastic.co> Co-Authored-By: Nicolas Ruflin <spam@ruflin.com>
Fields injected by docker autodiscover provider were being placed in alias fields introduced for ECS, change them to the new location and add selectors accordingly. This PR includes elastic#10862 and elastic#10758 As a summary: * Autodiscover selectors using ECS structure are added to autodiscover events, old selectors are kept for backwards compatibility * Autodiscover generated metadata follows ECS * Dedotting of labels is added, enabled by default, will be backported for 6.7, but disabled `docker.containers.labels` is not migrated, as it wasn't for `add_docker_metadata` (see elastic#9412) Fixes elastic#10757 Co-Authored-By: kaiyan-sheng <kaiyan.sheng@elastic.co> Co-Authored-By: Nicolas Ruflin <spam@ruflin.com> (cherry picked from commit 1bf8087)
Fields injected by docker autodiscover provider were being placed in alias fields introduced for ECS, change them to the new location and add selectors accordingly. This PR includes #10862 and #10758 As a summary: * Autodiscover selectors using ECS structure are added to autodiscover events, old selectors are kept for backwards compatibility * Autodiscover generated metadata follows ECS * Dedotting of labels is added, enabled by default, will be backported for 6.7, but disabled `docker.containers.labels` is not migrated, as it wasn't for `add_docker_metadata` (see #9412) Fixes #10757 (cherry picked from commit 1bf8087) Co-Authored-By: kaiyan-sheng <kaiyan.sheng@elastic.co> Co-Authored-By: Nicolas Ruflin <spam@ruflin.com>
Migrate the docker fields to ECS container fields. * docker.container.id -> container.id * docker.container.image -> container.image.name * docker.container.name -> container.name * docker.container.labels -> container.labels
Migrate the docker fields to ECS container fields.